Skip to content

[CAE-1088] feat: RESP3 notifications support #3418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

ndyakov
Copy link
Member

@ndyakov ndyakov commented Jun 26, 2025

This PR adds RESP3 push notifications support, enabling clients to handle server-initiated messages beyond traditional request-response patterns. The handlers for the notifications can be registered and deregistered by the end user. Added a special protected flag when registering a handler, so internal important handlers cannot be deregistered ( hitless upgrades will probably use this ).

Please review thoroughly. This is going to be executed as part of each read from connection and it is important it is performant both when there is a push notification to be processed and when there isn't one.

notes:

  • still can refactor error handling a bit.
  • should look at better timeout for checking the push notifications.

@ndyakov ndyakov requested a review from Copilot June 26, 2025 17:44
Copilot

This comment was marked as outdated.

ndyakov added 13 commits June 27, 2025 01:36
- Add PushNotificationRegistry for managing notification handlers
- Add PushNotificationProcessor for processing RESP3 push notifications
- Add client methods for registering push notification handlers
- Add PubSub integration for handling generic push notifications
- Add comprehensive test suite with 100% coverage
- Add push notification demo example

This system allows handling any arbitrary RESP3 push notification
with registered handlers, not just specific notification types.
- Change PushNotificationRegistry to allow only one handler per command
- RegisterHandler methods now return error if handler already exists
- Update UnregisterHandler to remove handler by command only
- Update all client methods to return errors for duplicate registrations
- Update comprehensive test suite to verify single handler behavior
- Add specific test for duplicate handler error scenarios

This prevents handler conflicts and ensures predictable notification
routing with clear error handling for registration conflicts.
- Remove all global push notification handler functionality
- Simplify registry to support only single handler per notification type
- Enable push notifications by default for RESP3 connections
- Update comprehensive test suite to remove global handler tests
- Update demo to show multiple specific handlers instead of global handlers
- Always respect custom processors regardless of PushNotifications flag

Push notifications are now automatically enabled for RESP3 and each
notification type has a single dedicated handler for predictable behavior.
- Add PushNotificationProcessor field to pool.Conn for connection-level processing
- Modify connection pool Put() and isHealthyConn() to handle push notifications
- Process pending push notifications before discarding connections
- Pass push notification processor to connections during creation
- Update connection pool options to include push notification processor
- Add comprehensive test for connection health check integration

This prevents connections with buffered push notification data from being
incorrectly discarded by the connection health check, ensuring push
notifications are properly processed and connections are reused.
…d clients

- Remove unused Timestamp and Source fields from PushNotificationInfo
- Add pushProcessor to newConn function to ensure Conn instances have push notifications
- Add push notification methods to Conn type for consistency
- Ensure cloned clients and Conn instances preserve push notification functionality

This fixes issues where:
1. PushNotificationInfo had unused fields causing confusion
2. Conn instances created via client.Conn() lacked push notification support
3. All client types now consistently support push notifications
- Add 10 new unit tests covering all previously untested code paths
- Test connection pool integration with push notifications
- Test connection health check integration
- Test Conn type push notification methods
- Test cloned client push notification preservation
- Test PushNotificationInfo structure validation
- Test edge cases and error scenarios
- Test custom processor integration
- Test disabled push notification scenarios

Total coverage now includes:
- 20 existing push notification tests
- 10 new comprehensive coverage tests
- All new code paths from connection pool integration
- All Conn methods and cloning functionality
- Edge cases and error handling scenarios
- Remove RegisterPushNotificationHandlerFunc methods from all types
- Remove PushNotificationHandlerFunc type adapter
- Keep only RegisterPushNotificationHandler method for cleaner interface
- Remove unnecessary push notification constants (keep only Redis Cluster ones)
- Update all tests to use simplified interface with direct handler implementations

Benefits:
- Cleaner, simpler API with single registration method
- Reduced code complexity and maintenance burden
- Focus on essential Redis Cluster push notifications only
- Users implement PushNotificationHandler interface directly
- No functional changes, just interface simplification
- Add sync.RWMutex to PushNotificationProcessor struct
- Protect enabled field access with read/write locks in IsEnabled() and SetEnabled()
- Use thread-safe IsEnabled() method in ProcessPendingNotifications()
- Fix concurrent access to enabled field that was causing data races

This resolves the race condition between goroutines calling IsEnabled() and
SetEnabled() concurrently, ensuring thread-safe access to the enabled field.
…ationName

- Add protected flag to RegisterHandler methods across all types
- Protected handlers cannot be unregistered, UnregisterHandler returns error
- Rename 'command' parameter to 'pushNotificationName' for clarity
- Update PushNotificationInfo.Command field to Name field
- Add comprehensive test for protected handler functionality
- Update all existing tests to use new protected parameter (false by default)
- Improve error messages to use 'push notification' terminology

Benefits:
- Critical handlers can be protected from accidental unregistration
- Clearer naming reflects that these are notification names, not commands
- Better error handling with informative error messages
- Backward compatible (existing handlers work with protected=false)
- Add VoidPushNotificationProcessor that reads and discards push notifications
- Create PushNotificationProcessorInterface for consistent behavior
- Always provide a processor (real or void) instead of nil
- VoidPushNotificationProcessor properly cleans RESP3 push notifications from buffer
- Remove all nil checks throughout codebase for cleaner, safer code
- Update tests to expect VoidPushNotificationProcessor when disabled

Benefits:
- Eliminates nil pointer risks throughout the codebase
- Follows null object pattern for safer operation
- Properly handles RESP3 push notifications even when disabled
- Consistent interface regardless of push notification settings
- Cleaner code without defensive nil checks everywhere
…ethods

- Remove enabled field from PushNotificationProcessor struct
- Remove IsEnabled() and SetEnabled() methods from processor interface
- Remove enabled parameter from NewPushNotificationProcessor()
- Update all interfaces in pool package to remove IsEnabled requirement
- Simplify processor logic - if processor exists, it works
- VoidPushNotificationProcessor handles disabled case by discarding notifications
- Update all tests to use simplified interface without enable/disable logic

Benefits:
- Simpler, cleaner interface with less complexity
- No unnecessary state management for enabled/disabled
- VoidPushNotificationProcessor pattern handles disabled case elegantly
- Reduced cognitive overhead - processors just work when set
- Eliminates redundant enabled checks throughout codebase
- More predictable behavior - set processor = it works
- Add nil check in newConn to create VoidPushNotificationProcessor when needed
- Fix tests to use Protocol 2 for disabled push notification scenarios
- Prevent nil pointer dereference in transaction and connection contexts
- Ensure consistent behavior across all connection creation paths

The panic was occurring because newConn could create connections with nil
pushProcessor when options didn't have a processor set. Now we always
ensure a processor exists (real or void) to maintain the 'never nil' guarantee.
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from 8bb0e59 to 8006fab Compare June 26, 2025 22:36
ndyakov added 3 commits June 27, 2025 01:47
- Add push processor initialization to NewSentinelClient to prevent nil pointer dereference
- Add GetPushNotificationProcessor and RegisterPushNotificationHandler methods to SentinelClient
- Use VoidPushNotificationProcessor for Sentinel (typically doesn't need push notifications)
- Ensure consistent behavior across all client types that inherit from baseClient

This fixes the panic that was occurring in Sentinel contexts where the pushProcessor
field was nil, causing segmentation violations when processing commands.
- Copy pushProcessor from parent client to transaction in newTx()
- Ensure transactions inherit push notification processor from parent client
- Prevent nil pointer dereference in transaction contexts (Watch, Unwatch, etc.)
- Maintain consistent push notification behavior across all Redis operations

This fixes the panic that was occurring in transaction examples where the
transaction's baseClient had a nil pushProcessor field, causing segmentation
violations during transaction operations like Watch and Unwatch.
- Add push processor initialization to NewFailoverClient to prevent nil pointer dereference
- Use VoidPushNotificationProcessor for failover clients (typically don't need push notifications)
- Ensure consistent behavior across all client creation paths including failover scenarios
- Complete the coverage of all client types that inherit from baseClient

This fixes the final nil pointer dereference that was occurring in failover client
contexts where the pushProcessor field was nil, causing segmentation violations
during Redis operations in sentinel-managed failover scenarios.
ndyakov added 4 commits June 27, 2025 13:59
…lation

- Add GetHandler() method to PushNotificationProcessorInterface for better encapsulation
- Add GetPushNotificationHandler() convenience method to Client and SentinelClient
- Remove HasHandlers() check from ProcessPendingNotifications to ensure notifications are always consumed
- Use PushNotificationProcessorInterface in internal pool package for proper abstraction
- Maintain GetRegistry() for backward compatibility and testing
- Update pubsub to use GetHandler() instead of GetRegistry() for cleaner code

Benefits:
- Better API encapsulation - no need to expose entire registry
- Cleaner interface - direct access to specific handlers
- Always consume push notifications from reader regardless of handler presence
- Proper abstraction in internal pool package
- Backward compatibility maintained
- Consistent behavior across all processor types
… FailoverClient

- Add PushNotifications field to FailoverOptions struct
- Update clientOptions() to pass PushNotifications field to Options
- Change SentinelClient and FailoverClient initialization to use same logic as regular Client
- Both clients now support real push notification processors when enabled
- Both clients use void processors only when explicitly disabled
- Consistent behavior across all client types (Client, SentinelClient, FailoverClient)

Benefits:
- SentinelClient and FailoverClient can now fully utilize push notifications
- Consistent API across all client types
- Real processors when push notifications are enabled
- Void processors only when explicitly disabled
- Equal push notification capabilities for all Redis client types
…better encapsulation

- Remove GetRegistry() method from PushNotificationProcessorInterface
- Enforce use of GetHandler() method for cleaner API design
- Add GetRegistryForTesting() method for test access only
- Update all tests to use new testing helper methods
- Maintain clean separation between public API and internal implementation

Benefits:
- Better encapsulation - no direct registry access from public interface
- Cleaner API - forces use of GetHandler() for specific handler access
- Consistent interface design across all processor types
- Internal registry access only available for testing purposes
- Prevents misuse of registry in production code
…anic

- Add nil check for proto.Reader parameter in both PushNotificationProcessor and VoidPushNotificationProcessor
- Prevent segmentation violation when ProcessPendingNotifications is called with nil reader
- Return early with nil error when reader is nil (graceful handling)
- Fix panic in TestProcessPendingNotificationsEdgeCases test

This addresses the runtime panic that occurred when rd.Buffered() was called on a nil reader,
ensuring robust error handling in edge cases where the reader might not be properly initialized.
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from 4e35707 to 9a7a5c8 Compare June 27, 2025 12:05
ndyakov added 4 commits June 27, 2025 16:27
- Remove unnecessary handlerWrapper complexity from push notifications
- Use separate maps for handlers and protection status in registry
- Store handlers directly without indirection layer
- Maintain same instance identity for registered/retrieved handlers
- Preserve all protected handler functionality with cleaner implementation

Changes:
- internal/pushnotif/registry.go: Use separate handlers and protected maps
- push_notifications.go: Remove handlerWrapper, store handlers directly
- Maintain thread-safe operations with simplified code structure

Benefits:
- Reduced memory overhead (no wrapper objects)
- Direct handler storage without type conversion
- Cleaner, more maintainable code
- Same functionality with better performance
- Eliminated unnecessary complexity layer
- Preserved all existing behavior and safety guarantees
- TestClientWithoutPushNotifications: Now expects error instead of nil
- TestClientPushNotificationEdgeCases: Now expects error instead of nil
…tions

- Fix TestConnWithoutPushNotifications to expect errors instead of nil
- Update test to verify error messages contain helpful information
- Add strings import for error message validation
- Maintain consistency with improved developer experience approach

The test now correctly expects errors when trying to register handlers on
connections with disabled push notifications, providing immediate feedback
to developers about configuration issues rather than silent failures.

This aligns with the improved developer experience where VoidProcessor
returns descriptive errors instead of silently ignoring registrations.
@ndyakov ndyakov marked this pull request as ready for review June 27, 2025 14:49
@ndyakov ndyakov changed the title [CAE-1088] resp3 notification handlers [CAE-1088] feat: RESP3 notifications support Jul 1, 2025
ndyakov added 2 commits July 2, 2025 17:04
Implement push notification processing in baseClient._getConn() to ensure
that all cluster topology changes are handled immediately before connections
are used for commands. This is critical for hitless upgrades and real-time
cluster state awareness.

Key Enhancements:

1. Enhanced Connection Retrieval (_getConn):
   - Process push notifications for both existing and new connections
   - Added processPushNotifications() call before returning connections
   - Ensures immediate handling of cluster topology changes
   - Proper error handling with connection removal on processing failures

2. Push Notification Processing Method:
   - Added processPushNotifications() method to baseClient
   - Only processes notifications for RESP3 connections with processors
   - Uses WithReader() to safely access connection reader
   - Integrates with existing push notification infrastructure

3. Connection Flow Enhancement:
   - Existing connections: Health check → Push notification processing → Return
   - New connections: Initialization → Push notification processing → Return
   - Failed processing results in connection removal and error return
   - Seamless integration with existing connection management

4. RESP3 Protocol Integration:
   - Protocol version check (only process for RESP3)
   - Push processor availability check
   - Graceful handling when processors are not available
   - Consistent behavior with existing push notification system

5. Error Handling and Recovery:
   - Remove connections if push notification processing fails
   - Return errors to trigger connection retry mechanisms
   - Maintain connection pool health and reliability
   - Prevent returning connections with unprocessed notifications

Implementation Details:
- processPushNotifications() checks protocol and processor availability
- Uses cn.WithReader() to safely access the connection reader
- Calls pushProcessor.ProcessPendingNotifications() for actual processing
- Applied to both pooled connections and newly initialized connections
- Consistent error handling across all connection retrieval paths

Flow Enhancement:
1. Connection requested via _getConn()
2. Connection retrieved from pool (existing or new)
3. Connection initialization (if new)
4. Push notification processing (NEW)
5. Connection returned to caller
6. Commands executed with up-to-date cluster state

Benefits:
- Immediate cluster topology awareness before command execution
- Enhanced hitless upgrade reliability with real-time notifications
- Reduced command failures during cluster topology changes
- Consistent push notification handling across all connection types
- Better integration with Redis cluster operations

This ensures that Redis cluster topology changes (MOVING, MIGRATING,
MIGRATED, FAILING_OVER, FAILED_OVER) are always processed before
connections are used, providing the foundation for reliable hitless
upgrades and seamless cluster operations.
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from 15a40c2 to 3eb71ea Compare July 4, 2025 15:09
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from 3eb71ea to 47dd490 Compare July 4, 2025 15:16
ndyakov added 6 commits July 4, 2025 19:53
Convert HandlerContext from struct to interface with strongly typed getters
for different client types. This provides better type safety and a cleaner
API for push notification handlers while maintaining flexibility.

Key Changes:

1. HandlerContext Interface Design:
   - Converted HandlerContext from struct to interface
   - Added strongly typed getters for different client types
   - GetClusterClient() returns ClusterClientInterface
   - GetSentinelClient() returns SentinelClientInterface
   - GetFailoverClient() returns FailoverClientInterface
   - GetRegularClient() returns RegularClientInterface
   - GetPubSub() returns PubSubInterface

2. Client Type Interfaces:
   - Defined ClusterClientInterface for cluster client access
   - Defined SentinelClientInterface for sentinel client access
   - Defined FailoverClientInterface for failover client access
   - Defined RegularClientInterface for regular client access
   - Defined PubSubInterface for pub/sub access
   - Each interface provides String() method for basic operations

3. Concrete Implementation:
   - Created handlerContext struct implementing HandlerContext interface
   - Added NewHandlerContext constructor function
   - Implemented type-safe getters with interface casting
   - Returns nil for incorrect client types (type safety)

4. Updated All Usage:
   - Updated Handler interface to use HandlerContext interface
   - Updated ProcessorInterface to use HandlerContext interface
   - Updated all processor implementations (Processor, VoidProcessor)
   - Updated all handler context creation sites
   - Updated test handlers and test context creation

5. Helper Methods:
   - Updated pushNotificationHandlerContext() in baseClient
   - Updated pushNotificationHandlerContext() in PubSub
   - Consistent context creation across all client types
   - Proper parameter passing for different connection types

6. Type Safety Benefits:
   - Handlers can safely cast to specific client types
   - Compile-time checking for client type access
   - Clear API for accessing different client capabilities
   - No runtime panics from incorrect type assertions

7. API Usage Example:
   ```go
   func (h *MyHandler) HandlePushNotification(
       ctx context.Context,
       handlerCtx HandlerContext,
       notification []interface{},
   ) bool {
       // Strongly typed access
       if clusterClient := handlerCtx.GetClusterClient(); clusterClient != nil {
           // Handle cluster-specific logic
       }
       if sentinelClient := handlerCtx.GetSentinelClient(); sentinelClient != nil {
           // Handle sentinel-specific logic
       }
       return true
   }
   ```

8. Backward Compatibility:
   - Interface maintains same functionality as original struct
   - All existing handler patterns continue to work
   - No breaking changes to handler implementations
   - Smooth migration path for existing code

Benefits:
- Strong type safety for client access in handlers
- Clear API with explicit client type getters
- Compile-time checking prevents runtime errors
- Flexible interface allows future extensions
- Better separation of concerns between client types
- Enhanced developer experience with IntelliSense support

This enhancement provides handlers with strongly typed access to different
Redis client types while maintaining the flexibility and context information
needed for sophisticated push notification handling, particularly important
for hitless upgrades and cluster management operations.
…main package

Move push notification handler and context interfaces to main package to enable
strongly typed getters using concrete Redis client types instead of interfaces.
This provides much better type safety and usability for push notification handlers.

Key Changes:

1. Main Package Implementation:
   - Moved PushNotificationHandlerContext to push_notifications.go
   - Moved PushNotificationHandler to push_notifications.go
   - Implemented concrete types for all getters
   - GetClusterClient() returns *ClusterClient
   - GetSentinelClient() returns *SentinelClient
   - GetRegularClient() returns *Client
   - GetPubSub() returns *PubSub

2. Concrete Type Benefits:
   - No need for interface definitions or type assertions
   - Direct access to concrete client methods and properties
   - Compile-time type checking with actual client types
   - IntelliSense support for all client-specific methods
   - No runtime panics from incorrect type casting

3. Handler Interface with Concrete Types:
   ```go
   type PushNotificationHandlerContext interface {
       GetClusterClient() *ClusterClient
       GetSentinelClient() *SentinelClient
       GetRegularClient() *Client
       GetPubSub() *PubSub
       GetConn() *pool.Conn
       IsBlocking() bool
   }
   ```

4. Adapter Pattern Implementation:
   - Created handlerAdapter to bridge internal and public interfaces
   - Created voidProcessorAdapter for void processor functionality
   - Seamless conversion between internal and public contexts
   - Maintains compatibility with existing internal architecture

5. Context Conversion Functions:
   - convertInternalToPublicContext() for seamless conversion
   - Proper context bridging between internal and public APIs
   - Maintains all context information during conversion
   - Consistent behavior across all client types

6. Updated All Integration Points:
   - Updated redis.go to use public context conversion
   - Updated pubsub.go to use public context conversion
   - Updated sentinel.go to use void processor adapter
   - Maintained backward compatibility with existing code

7. Handler Usage Example:
   ```go
   func (h *MyHandler) HandlePushNotification(
       ctx context.Context,
       handlerCtx PushNotificationHandlerContext,
       notification []interface{},
   ) bool {
       // Direct access to concrete types - no casting needed!
       if clusterClient := handlerCtx.GetClusterClient(); clusterClient != nil {
           // Full access to ClusterClient methods
           nodes := clusterClient.ClusterNodes(ctx)
           // ... cluster-specific logic
       }

       if regularClient := handlerCtx.GetRegularClient(); regularClient != nil {
           // Full access to Client methods
           info := regularClient.Info(ctx)
           // ... regular client logic
       }

       return true
   }
   ```

8. Type Safety Improvements:
   - No interface{} fields in public API
   - Concrete return types for all getters
   - Compile-time verification of client type usage
   - Clear API with explicit client type access
   - Enhanced developer experience with full type information

Benefits:
- Strongly typed access to concrete Redis client types
- No type assertions or interface casting required
- Full IntelliSense support for client-specific methods
- Compile-time type checking prevents runtime errors
- Clean public API with concrete types
- Seamless integration with existing internal architecture
- Enhanced developer experience and productivity

This implementation provides handlers with direct access to concrete Redis
client types while maintaining the flexibility and context information needed
for sophisticated push notification handling, particularly important for
hitless upgrades and cluster management operations.
… adapters

Consolidate all push notification handling logic in the root package to eliminate
adapters and simplify the architecture. This provides direct access to concrete
types without any intermediate layers or type conversions.

Key Changes:

1. Moved Core Types to Root Package:
   - Moved Registry, Processor, VoidProcessor to push_notifications.go
   - Moved all push notification constants to root package
   - Removed internal/pushnotif package dependencies
   - Direct implementation without internal abstractions

2. Eliminated All Adapters:
   - Removed handlerAdapter that bridged internal and public interfaces
   - Removed voidProcessorAdapter for void processor functionality
   - Removed convertInternalToPublicContext conversion functions
   - Direct usage of concrete types throughout

3. Simplified Architecture:
   - PushNotificationHandlerContext directly implemented in root package
   - PushNotificationHandler directly implemented in root package
   - Registry, Processor, VoidProcessor directly in root package
   - No intermediate layers or type conversions needed

4. Direct Type Usage:
   - GetClusterClient() returns *ClusterClient directly
   - GetSentinelClient() returns *SentinelClient directly
   - GetRegularClient() returns *Client directly
   - GetPubSub() returns *PubSub directly
   - No interface casting or type assertions required

5. Updated All Integration Points:
   - Updated redis.go to use direct types
   - Updated pubsub.go to use direct types
   - Updated sentinel.go to use direct types
   - Removed all internal/pushnotif imports
   - Simplified context creation and usage

6. Core Implementation in Root Package:
   ```go
   // Direct implementation - no adapters needed
   type Registry struct {
       handlers  map[string]PushNotificationHandler
       protected map[string]bool
   }

   type Processor struct {
       registry *Registry
   }

   type VoidProcessor struct{}
   ```

7. Handler Context with Concrete Types:
   ```go
   type PushNotificationHandlerContext interface {
       GetClusterClient() *ClusterClient    // Direct concrete type
       GetSentinelClient() *SentinelClient  // Direct concrete type
       GetRegularClient() *Client           // Direct concrete type
       GetPubSub() *PubSub                  // Direct concrete type
   }
   ```

8. Comprehensive Test Suite:
   - Added push_notifications_test.go with full test coverage
   - Tests for Registry, Processor, VoidProcessor
   - Tests for HandlerContext with concrete type access
   - Tests for all push notification constants
   - Validates all functionality works correctly

9. Benefits:
   - Eliminated complex adapter pattern
   - Removed unnecessary type conversions
   - Simplified codebase with direct type usage
   - Better performance without adapter overhead
   - Cleaner architecture with single source of truth
   - Enhanced developer experience with direct access

10. Architecture Simplification:
    Before: Client -> Adapter -> Internal -> Adapter -> Handler
    After:  Client -> Handler (direct)

    No more:
    - handlerAdapter bridging interfaces
    - voidProcessorAdapter for void functionality
    - convertInternalToPublicContext conversions
    - Complex type mapping between layers

This refactoring provides a much cleaner, simpler architecture where all
push notification logic lives in the root package with direct access to
concrete Redis client types, eliminating unnecessary complexity while
maintaining full functionality and type safety.
Remove internal/pushprocessor and pushnotif packages that contained duplicate
and unresolved types. All push notification functionality is now consolidated
in the root package with direct type resolution.

Removed Packages:
- internal/pushprocessor/ - contained duplicate Registry, Processor, VoidProcessor
- pushnotif/ - contained interface wrappers that are no longer needed

Benefits:
- Single source of truth for all push notification logic
- No duplicate implementations or unresolved type references
- Cleaner codebase with all functionality in root package
- Eliminated confusion between internal and public interfaces
- Simplified architecture with direct type usage

All functionality remains intact and tests pass. The root package now contains
the complete, self-contained push notification implementation with concrete
types and no external dependencies.
Split push notification implementation into focused, maintainable files for
better code organization and easier navigation. Each file now has a clear
responsibility and contains related functionality.

File Organization:

1. push_notifications.go (Main API):
   - Push notification constants (MOVING, MIGRATING, etc.)
   - PushNotificationHandler interface
   - PushNotificationProcessorInterface
   - Public API wrappers (PushNotificationRegistry, PushNotificationProcessor)
   - Main entry point for push notification functionality

2. push_notification_handler_context.go (Context):
   - PushNotificationHandlerContext interface
   - pushNotificationHandlerContext concrete implementation
   - NewPushNotificationHandlerContext constructor
   - All context-related functionality with concrete type getters

3. push_notification_processor.go (Core Logic):
   - Registry implementation for handler management
   - Processor implementation for notification processing
   - VoidProcessor implementation for RESP2 connections
   - Core processing logic and notification filtering

Benefits:
- Clear separation of concerns between files
- Easier to navigate and maintain codebase
- Focused files with single responsibilities
- Better code organization for large codebase
- Simplified debugging and testing

File Responsibilities:
- Main API: Public interfaces and constants
- Context: Handler context with concrete type access
- Processor: Core processing logic and registry management

All functionality remains intact with improved organization. Tests pass
and compilation succeeds with the new file structure.
@@ -77,6 +77,7 @@ func (cn *Conn) WithReader(
return err
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this line

@@ -228,6 +232,7 @@ func (p *ConnPool) dialConn(ctx context.Context, pooled bool) (*Conn, error) {

cn := NewConn(netConn)
cn.pooled = pooled

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this line

@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from 493ec66 to 604c8e3 Compare July 5, 2025 02:04
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from e96c873 to b23f43c Compare July 5, 2025 03:21
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from 7b883d0 to 7a0f316 Compare July 5, 2025 10:09
@ndyakov ndyakov force-pushed the ndyakov/CAE-1088-resp3-notification-handlers branch from af78f0d to 225c0bf Compare July 5, 2025 10:34
@ndyakov ndyakov requested a review from Copilot July 5, 2025 10:51
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enables RESP3 push notifications support by integrating a push notification processor across clients, connections, and pools, and exposing handler registration APIs.

  • Introduce NotificationProcessor, handler registry, and default/void processor implementations
  • Hook processPushNotifications into client/connection lifecycle for RESP3
  • Expose RegisterPushNotificationHandler and GetPushNotificationHandler on Client, SentinelClient, and Conn

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tx.go Copy pushProcessor into new transactions
sentinel.go Initialize pushProcessor in sentinel clients and add push handler registration/get methods
redis.go Inject pushProcessor into baseClient, hook notification processing into all commands, and add client-level API
options.go Add PushNotificationProcessor option and pass Protocol to pool configuration
push_notifications.go Define push notification constants and processor factory functions
push/registry.go Implement handler registry with protected handlers
push/processor.go Define NotificationProcessor, Processor, and VoidProcessor logic
internal/proto/reader.go Add PeekPushNotificationName to pre-read notification names
internal/pool/pool.go Adjust pool to recognize buffered RESP3 push markers
pubsub.go Integrate push notification pre-read in PubSub
Comments suppressed due to low confidence (2)

redis.go:902

  • You expose Get and Register methods but no UnregisterPushNotificationHandler on Client. Consider adding an UnregisterPushNotificationHandler to allow clients to remove handlers when needed.
func (c *Client) GetPushNotificationHandler(pushNotificationName string) push.NotificationHandler {

internal/proto/reader.go:93

  • The implementation calls util.BytesToString(buf) but util is not imported, causing a compilation error. Add import "github.com/redis/go-redis/v9/internal/util" or use string(buf) to convert the bytes.
func (r *Reader) PeekPushNotificationName() (string, error) {

Comment on lines +393 to +394
return
}
Copy link
Preview

Copilot AI Jul 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This early return in Put prevents the connection from being placed back into the pool, leading to connection leaks. You should still call p.connsMu/p.Put or similar logic after detecting push data, or explicitly return the connection to the pool.

Suggested change
return
}
}
// Allow the connection to proceed to the pool management logic

Copilot uses AI. Check for mistakes.

// Use WithReader to access the reader and process push notifications
// This is critical for hitless upgrades to work properly
// NOTE: almost no timeouts are set for this read, so it should not block
return cn.WithReader(ctx, 1, func(rd *proto.Reader) error {
Copy link
Preview

Copilot AI Jul 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a timeout of 1 (nanosecond) to WithReader will almost always time out immediately. Use a proper time.Duration (e.g., 0 to disable timeout or c.opt.ReadTimeout) to avoid unintended errors.

Suggested change
return cn.WithReader(ctx, 1, func(rd *proto.Reader) error {
return cn.WithReader(ctx, 0, func(rd *proto.Reader) error {

Copilot uses AI. Check for mistakes.

push/errors.go Outdated
Comment on lines +121 to +124
if err == nil {
return false
}
return fmt.Sprintf("%v", err) == fmt.Sprintf(MsgHandlerExists, extractNotificationName(err))
Copy link
Preview

Copilot AI Jul 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsHandlerExistsError uses a string comparison with a placeholder and extractNotificationName always returns "unknown", so it will never correctly detect the real error. Consider using errors.Is or inspecting a typed HandlerError to reliably identify this case.

Suggested change
if err == nil {
return false
}
return fmt.Sprintf("%v", err) == fmt.Sprintf(MsgHandlerExists, extractNotificationName(err))
var handlerErr *HandlerError
if errors.As(err, &handlerErr) {
return handlerErr.Message == "cannot overwrite existing handler for push notification"
}
return false

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant